Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed: workspace entries should not be used when trying to check for peer dependencies #7812

Closed
wants to merge 1 commit into from
Closed

Fixed: workspace entries should not be used when trying to check for peer dependencies #7812

wants to merge 1 commit into from

Conversation

antoine-malliarakis
Copy link

@antoine-malliarakis antoine-malliarakis commented Jan 10, 2020 •
edited
Loading

Fixed: the package requests introduced by workspace mechanism should be excluded from the scope of requests checked for usage
Added peer tests (all credits go to @hulkish for the initial test cases)

Summary

The idea of this PR is to fix the following behaviour which were currently giving tons of "false positive" warnings about unused peer dependencies in workspaces context.

  • Current state:

  • say you have a project A within your workspace which specifies a peer dependency on a package which is outside the workspace. Currently you get a peer unmet warning.

  • say you have a project B within your workspace which specifies a peer dependency on another project C which is inside the workspace and that your root project declares a dependency on B without a dependency on C. Currently you don't get a peer unmet warning.

This relates (somehow) to issue #5810

Test plan

Simple missing peer dependency warning (prod)

Create a root project with three subprojects, a, b and c (we'll be using version 1.0 everywhere here).

  • The root project has no dependency
  • a has b in its peer dependencies.
  • c has a in its prod dependencies

Run yarn install
Expected: a warning for c missing a peer dependency on b.

Simple missing peer dependency warning (dev)

Create a root project with three subprojects, a, b and c (we'll be using version 1.0 everywhere here).

  • The root project has no dependency
  • a has b in its peer dependencies.
  • c has a in its dev dependencies

Run yarn install
Expected: a warning for c missing a peer dependency on b.

Simple unmet peer dependency warning (prod)

Create a root project with three subprojects, a, b and c (we'll be using version 1.0 everywhere here).

  • The root project has no dependency
  • a has b@0.5 in its peer dependencies.
  • c has a and b@1.0 in its prod dependencies

Run yarn install
Expected: a warning for c with an unmet peer dependency on b@0.5.

Simple unmet peer dependency warning (dev)

Create a root project with three subprojects, a, b and c (we'll be using version 1.0 everywhere here).

  • The root project has no dependency
  • a has b@0.5 in its peer dependencies.
  • c has a and b@1.0 in its dev dependencies

Run yarn install
Expected: a warning for c with an unmet a peer dependency on b@0.5.

No peer dependency issue

Create a root project with three subprojects, a, b and c (we'll be using version 1.0 everywhere here).

  • The root project has no dependency
  • a has b in its peer dependencies.
  • c has a and b in its prod dependencies

Run yarn install
Expected: no warning

@antoine-malliarakis antoine-malliarakis force-pushed the fix/workspaces/peer-dependencies-for-workspace-packages branch from 56934b5 to 0514589 Compare January 14, 2020 08:02
@buildsize
Copy link

buildsize bot commented Jan 14, 2020 •
edited
Loading

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.18 MB 1.18 MB 385 bytes (0%)
yarn-[version].js 4.86 MB 4.86 MB 2.86 KB (0%)
yarn-legacy-[version].js 5.06 MB 5.06 MB 3.6 KB (0%)
yarn-v[version].tar.gz 1.19 MB 1.19 MB 1020 bytes (0%)
yarn_[version]all.deb 870.6 KB 870.8 KB 198 bytes (0%)

@antoine-malliarakis antoine-malliarakis marked this pull request as ready for review January 14, 2020 08:13
@antoine-malliarakis antoine-malliarakis changed the title Fixed: the reference tree selected should not be looked for among wor… Fixed: workspace entries should not be used when trying to check for peer dependencies Jan 14, 2020
@bk3c bk3c mentioned this pull request Jan 16, 2020
Closed
@antoine-malliarakis antoine-malliarakis force-pushed the fix/workspaces/peer-dependencies-for-workspace-packages branch from 0514589 to bb65cf2 Compare February 4, 2020 14:24
@antoine-malliarakis
Copy link
Author

For what it's worth the failing test seems to be also failing on the master branch.

antoine-malliarakis
antoine-malliarakis commented Feb 4, 2020
View reviewed changes
src/package-linker.js Outdated
}

const {refTree, peerError, resolvedPeerPkg} = refTreesStatus[0];
if (resolvedPeerPkg) {
ref.addDependencies(resolvedPeerPkg.patterns);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the first reference tree entry may see it's corresponding dependency added to the dependencies if resolved.

Other contributions will be discarded and only warnings will be issued if some of them are not resolved.

src/package-linker.js Outdated
resolvedPeerPkg = peerPkgRef;
} else {
peerError = 'incorrectPeer';
const refTreesStatus = [];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We start by building an array containing the results on a per reference tree basis.
The first one would be the first reference tree found which (by construction) would be the first defined of the following:

  • dependency defined at the root level
  • first package (according to the platform's sorting conventions) which defines a dependency on that package

src/package-linker.js Outdated

for (const peerDepName in peerDeps) {
const range = peerDeps[peerDepName];
const meta = peerDepsMeta && peerDepsMeta[peerDepName];

const isOptional = !!(meta && meta.optional);

const onNotResolved = isOptional
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to do with optional dependencies when they're not resolved.

src/package-linker.js Outdated
minDistance = distance;
}
}
const refTrees = extractRefTrees(ref);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we build an array of "reference trees" which would be sorted by "name" (the workspace's root reference tree carrying the empty name) and where each entry would represent the corresponding dependency path, instead of only taking the first one which was hanging around.

@antoine-malliarakis
Copy link
Author

Just wondering if there was anything I should do here to proceed to the next stage (or to have it rejected with corresponding feedbacks for what it's worth).

@antoine-malliarakis
Copy link
Author

Hello @rally25rs

Sorry for bothering you but I was wondering: is there anything I could do in order to ease the follow up on this PR ? I mean, is it even still relevant to have such a PR with yarn 2 "around the corner" ? Or did I miss something ?

Thanks and all apologize for any inconvenience caused

Regards

rally25rs
rally25rs approved these changes Feb 22, 2020
View reviewed changes
Copy link
Contributor

@rally25rs rally25rs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoine-malliarakis this is nice, thanks for the contribution! 👍
Tests pass locally, so I'm not sure what's up with CircleCI... I feel like any time I submit a PR they fail too 😆

I left a small comment on the test.

package-linker is feeling fairly cluttered, especially with adding another set of tree-traversal logic... if you wanted to put some extra effort into refactoring it off to it's own helper module that'd be nice, but overall 👍

I know with yarn v2 here we aren't making a lot of yarn v1 changes, but from the linked issue this seems like it's a pretty big/common annoyance so I'd be in favor of getting in merged.

@arcanis any additional thoughts?

@antoine-malliarakis antoine-malliarakis force-pushed the fix/workspaces/peer-dependencies-for-workspace-packages branch from bb65cf2 to 15665db Compare February 24, 2020 10:28
@arcanis
Copy link
Member

arcanis commented Feb 24, 2020

I'll let you decide, but to be honest I'm not sure it's a good idea to merge "significant" changes to the linker. I have no idea what side effect it may have 😕

I think the main problem I can see is that afaik this case wasn't considered when implementing workspaces (even in the 2.x I didn't think about making peer dependency implicitly define a regular dependency for workspaces), so it's possible that some parts of the application expect the workspace dependencies to be explicitly listed not only as peer dependencies, but also dev dependencies.

@arcanis arcanis closed this Feb 24, 2020
@arcanis arcanis reopened this Feb 24, 2020
@arcanis
Copy link
Member

arcanis commented Feb 24, 2020

(Oops sorry, didn't mean to close 😅)

@antoine-malliarakis
Copy link
Author

I'll let you decide, but to be honest I'm not sure it's a good idea to merge "significant" changes to the linker. I have no idea what side effect it may have 😕

I think the main problem I can see is that afaik this case wasn't considered when implementing workspaces (even in the 2.x I didn't think about making peer dependency implicitly define a regular dependency for workspaces), so it's possible that some parts of the application expect the workspace dependencies to be explicitly listed not only as peer dependencies, but also dev dependencies.

Would you rather, therefore, have this dev been done only on the v2 codeline ? (Would seem fair enough, to be honest)

@vpicone
Copy link

vpicone commented Mar 4, 2020

Would love for this feature to be released in v1. The peer-dep flags can get really rough when working with workspaces!
Screen Shot 2020-03-04 at 3 19 12 PM

@antoine-malliarakis
Copy link
Author

Would love for this feature to be released in v1. The peer-dep flags can get really rough when working with workspaces!
Screen Shot 2020-03-04 at 3 19 12 PM

Actually what this change will fix is those kind of warnings:

image

It may even add new warnings (because it will basically ensure that workspace peer dependencies are appropriately checked).

But I'm all in for advices ;)

@antoine-malliarakis antoine-malliarakis force-pushed the fix/workspaces/peer-dependencies-for-workspace-packages branch from 15665db to e89e37c Compare March 9, 2020 09:53
Fixed: the reference tree selected should not be looked for among wor…
5e22d2f 
…kspaces virtual entries

Fixed: the package requests introduced by workspace mechanism should be excluded from the scope of requests checked for usage
Added peer tests (all credits go to @hulkish for the initial test cases)
@antoine-malliarakis antoine-malliarakis force-pushed the fix/workspaces/peer-dependencies-for-workspace-packages branch from e89e37c to 5e22d2f Compare May 28, 2020 15:48
@antoine-malliarakis
Copy link
Author

I'll let you decide, but to be honest I'm not sure it's a good idea to merge "significant" changes to the linker. I have no idea what side effect it may have 😕
I think the main problem I can see is that afaik this case wasn't considered when implementing workspaces (even in the 2.x I didn't think about making peer dependency implicitly define a regular dependency for workspaces), so it's possible that some parts of the application expect the workspace dependencies to be explicitly listed not only as peer dependencies, but also dev dependencies.

Would you rather, therefore, have this dev been done only on the v2 codeline ? (Would seem fair enough, to be honest)

@arcanis Actually I just checked and it seems that there is no such issue in yarn 2... So there would be no real need to try "redoing it" on the 2.x codeline.

@antoine-malliarakis
Copy link
Author

Closing this MR as "out of touch" (lost track, no longer using yarn v1, and may have more impacts than foreseen)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rally25rs rally25rs rally25rs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@antoine-malliarakis @arcanis @vpicone @rally25rs